Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Global connection context #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Global connection context #227

wants to merge 1 commit into from

Conversation

snoyberg
Copy link
Owner

Pinging @sjakobi and @vincenthz. This is intended to speed up manager creation (#214). However, I'm not sure how safe it is to have a global connection context like this. @vincenthz can you weigh in on this approach? Once I get an idea of the semantics of this, I'll clean up this commit and ask for a second review.

@vincenthz
Copy link
Contributor

vincenthz commented Sep 13, 2016 •
edited
Loading

yes, this is supposed to be much faster, as the system store would be loaded only once, and shared by all subsequent TLS connections. I haven't checked that it works under a unsafePerformIO context, but I don't see any reason why not. One thing that I'm worried about with this, is whether or not there's going to be any lazy IO pending until the certificate store is used.

@snoyberg
Copy link
Owner Author

As I've written the code, there definitely will be laziness. The question is whether this is a good thing or not. Options:

  1. When calling newManager, force the evaluation of the global context. This ensures that we deterministically perform that action when creating the manager. We'd likely want to apply strictness to getGlobalManager too then. This would ensure that IO is only performed in an explicit IO call.
  2. Leave as-is, and the first time we make an HTTPS connection, the thunk will be forced, making the first connection taking longer than subsequent connections. On the other hand: no overhead at all if we only make insecure connections. Only other concern is if multiple threads are accessing the thunk at the same time, but in practice GHC's runtime should make that situation OK.

@vincenthz
Copy link
Contributor

vincenthz commented Sep 13, 2016 •
edited
Loading

I think it would make sense that the whole of the certificate store is strictly loaded once, only after it's been really needed (so lazy from the outside point of view, but strict inside); I think that's the best outcome, as it still allow fast HTTP without needing the cert store. But I haven't checked that the certificate store loading does the right thing here.

Blaisorblade
Blaisorblade reviewed Sep 13, 2016
View reviewed changes
http-client-tls/Network/HTTP/Client/TLS.hs
@@ -122,7 +134,9 @@ convertConnection conn = makeConnection

-- | Evil global manager, to make life easier for the common use case
globalManager :: IORef Manager
globalManager = unsafePerformIO (newManager tlsManagerSettings >>= newIORef)
globalManager = unsafePerformIO $ do
man <- newManager $ mkManagerSettingsContext (Just globalContext) def Nothing
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this hunk is just expanding >>= to do-notation (fine) and inlining/copy-pasting tlsManagerSettings—why the inlining? If there's a tricky good reason, a comment would be useful, lest somebody tries removing the duplication.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a tricky reason: my lack of clarity. I'm not sure if all of the changes I applied here should go through. I was originally planning on just modifying globalManager and not implementing the globalContext, and then I took it a step further and defaulted to everyone sharing a globalContext. Once we're sure that we can use globalContext, I'd revert this hunk. Otherwise, I'd get rid of globalContext and inline the init call to here.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes I was wondering. I compulsively use separate commits for such separate steps (so the choice reduces to cherry-picking), but I totally realize that's usually overkill 😅

@snoyberg
Copy link
Owner Author

@vincenthz Just to confirm, I should hold off on merging this PR (or any version of it) until you've had a chance to check that, right?

@snoyberg snoyberg mentioned this pull request Sep 15, 2016
Closed
@snoyberg snoyberg force-pushed the avoid-extra-context-init branch from 436ac32 to 1848e64 Compare September 15, 2016 16:03
@snoyberg snoyberg force-pushed the avoid-extra-context-init branch from 1848e64 to e96f049 Compare September 15, 2016 16:09
@ocheron
Copy link
Contributor

ocheron commented Oct 31, 2016

From an API standpoint it would be more general to have:

mkManagerSettingsContext
    :: IO NC.ConnectionContext
    -> NC.TLSSettings
    -> Maybe NC.SockSettings
    -> ManagerSettings

ManagerSettings is the receipe used by the manager to build connections. With this API I can supply an IO-based function to decide exactly what to do:

  • allocate a new ConnectionContext for each new Manager
  • reuse a global ConnectionContext until it is too old and must be recycled
  • reuse a global ConnectionContext indefinitely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@snoyberg @vincenthz @ocheron @Blaisorblade